fix(dptest): Wrong dptest results except for energy head #4280
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)source/tests/pt/test_dp_test.py (2)
This change introduces a deliberate permutation of the type_map to validate that dptest correctly handles type mappings from input.json, which directly addresses the issue #4249 where type_map wasn't being properly updated for non-DeepPot models. Line range hint Setting Let's verify that this test case covers the type_map handling issue: ✅ Verification successfulProperty tests correctly handle atomic parameter variations The verification shows that:
The atomic parameter handling is well-tested across the codebase, and this change appropriately extends the test coverage. 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if there are other test cases that might need similar atomic=True setting
# Look for other property test cases that might need updating
rg -l "class.*Test.*Property" source/tests/
Length of output: 342 Script: #!/bin/bash
# Check for similar property test patterns and atomic parameter usage
rg -A 5 -B 5 "test.*property" source/tests/pt/test_dp_test.py source/tests/pt/model/test_property_fitting.py source/tests/consistent/fitting/test_property.py
# Also check for atomic parameter usage in these files
rg -A 2 -B 2 "atomic\s*=\s*(True|False)" source/tests/pt/test_dp_test.py source/tests/pt/model/test_property_fitting.py source/tests/consistent/fitting/test_property.py
Length of output: 4113 Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
deepmd/entrypoints/test.py (1)
Line range hint 182-187: Consider raising an error for unsupported models.
While the warning message is helpful, allowing an unsupported model to continue processing might lead to incorrect results without the user noticing.
Consider either:
- Raising an error to prevent using unsupported models:
elif isinstance(dp, DeepGlobalPolar): # should not appear in this new version
- log.warning(
- "Global polar model is not currently supported. Please directly use the polar mode and change loss parameters."
- )
- err = test_polar(
- dp, data, numb_test, detail_file, atomic=False
- ) # YWolfeee: downward compatibility
+ raise ValueError(
+ "Global polar model is not supported in this version. Please use the DeepPolar model instead and adjust loss parameters accordingly."
+ )- Or add a stronger warning about potential incorrect results:
elif isinstance(dp, DeepGlobalPolar): # should not appear in this new version
log.warning(
- "Global polar model is not currently supported. Please directly use the polar mode and change loss parameters."
+ "Global polar model is not currently supported and may produce incorrect results. Please use the DeepPolar model instead and adjust loss parameters accordingly."
)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4280 +/- ##
=======================================
Coverage 84.29% 84.29%
=======================================
Files 553 553
Lines 51820 51820
Branches 3052 3052
=======================================
+ Hits 43683 43684 +1
+ Misses 7177 7175 -2
- Partials 960 961 +1 ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
|
Maybe who can help me rerun the failed UT? I do not have the permission. I don't think it is a code problem. |
|
Fixed. |
Solve issue #4249
In
/deepmd/entrypoints/test.pyline 127tmap = dp.get_type_map() if isinstance(dp, DeepPot) else None. If we useDeepPropertyorDeepPolarorDeepDOS.....tmapis None. Sotype_mapis not modified again according totype_mapininput.json. Soatypeis wrong in the model forward process. The model prediction value is wrong.According to @njzjz , It seems that in the
r2branch,get_type_map()is only available inDeepPot. After we refactorDeepEvalin v3, this should not be a problem anymore.I also change the order of
type_mapin UT to ensure that whentype_mapofinput.jsondoesn't match thetype_mapof data, the result of the dptest is still correct.Summary by CodeRabbit
New Features
DeepGlobalPolarmodel usage, recommending theDeepPolarmodel instead.Bug Fixes
Documentation